fix(aci milestone 3): fake IDs in serializer if new models don't have old model equivalents #101974
Conversation
github-actions
bot
added
the
Scope: Backend
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
src/sentry/incidents/endpoints/serializers/workflow_engine_action.py
Outdated
Show resolved
Hide resolved
| alert_rule_id=alert_rule_id | ||
| ) | ||
| except AlertRuleDetector.DoesNotExist: | ||
| detector_id = int(alert_rule_id) - OFFSET |
There was a problem hiding this comment.
This operation is a bijection so fetching the detector ID works out nicely :)
![semgrep-code-getsentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| alert_rule_id = AlertRuleDetector.objects.values_list( | ||
| "alert_rule_id", flat=True | ||
| ).get(detector=obj) |
There was a problem hiding this comment.
Risk: Affected versions of Django are vulnerable to Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection'). SQL injection in Django's ORM column aliases: when using QuerySet.annotate(), QuerySet.alias(), QuerySet.aggregate(), or QuerySet.extra() with dictionary expansion (**kwargs), the dictionary keys are used unescaped as SQL column aliases. On MySQL and MariaDB backends, an attacker who can influence those keys (for example, by passing a crafted dict of annotations) can inject arbitrary SQL into the generated query.
Manual Review Advice: A vulnerability from this advisory is reachable if you are using Django with MySQL or MariaDB
Fix: Upgrade this library to at least version 5.2.7 at sentry/uv.lock:305.
Reference(s): GHSA-hpr9-3m2g-3j9p, CVE-2025-59681
🎉 Fixed in commit 8c89442 🎉
| alert_rule_trigger_id = DataConditionAlertRuleTrigger.objects.values_list( | ||
| "alert_rule_trigger_id", flat=True | ||
| ).get(data_condition=detector_trigger) |
There was a problem hiding this comment.
High severity vulnerability may affect your project—review required:
Line 59 lists a dependency (django) with a known High severity vulnerability.
ℹ️ Why this matters
Affected versions of Django are vulnerable to Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection'). SQL injection in Django's ORM column aliases: when using QuerySet.annotate(), QuerySet.alias(), QuerySet.aggregate(), or QuerySet.extra() with dictionary expansion (**kwargs), the dictionary keys are used unescaped as SQL column aliases. On MySQL and MariaDB backends, an attacker who can influence those keys (for example, by passing a crafted dict of annotations) can inject arbitrary SQL into the generated query.
To resolve this comment:
Check if you are using Django with MySQL or MariaDB.
- If you're affected, upgrade this dependency to at least version 5.2.7 at uv.lock.
- If you're not affected, comment
/fp we don't use this [condition]
💬 Ignore this finding
To ignore this, reply with:
/fp <comment>for false positive/ar <comment>for acceptable risk/other <comment>for all other reasons
You can view more details on this finding in the Semgrep AppSec Platform here.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #101974 +/- ##
===========================================
+ Coverage 80.97% 80.98% +0.01%
===========================================
Files 8729 8740 +11
Lines 388412 388830 +418
Branches 24628 24628
===========================================
+ Hits 314502 314907 +405
- Misses 73550 73563 +13
Partials 360 360 |
There was a problem hiding this comment.
Seems reasonable to me, but I think the value of OFFSET should be set once and documented, and potentially abstracted via some trivial helper functions.
Bonus points if we use them to improve type safety (we don't really have well-typed ID types, so probably not).
src/sentry/incidents/endpoints/serializers/workflow_engine_action.py
Outdated
Show resolved
Hide resolved
src/sentry/incidents/endpoints/serializers/workflow_engine_action.py
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| alert_rule_id = AlertRuleDetector.objects.values_list( | ||
| "alert_rule_id", flat=True | ||
| ).get(detector=obj) |
There was a problem hiding this comment.
Risk: Affected versions of Django are vulnerable to Improper Neutralization of Special Elements used in an SQL Command ('SQL Injection'). SQL injection in Django's ORM column aliases: when using QuerySet.annotate(), QuerySet.alias(), QuerySet.aggregate(), or QuerySet.extra() with dictionary expansion (**kwargs), the dictionary keys are used unescaped as SQL column aliases. On MySQL and MariaDB backends, an attacker who can influence those keys (for example, by passing a crafted dict of annotations) can inject arbitrary SQL into the generated query.
Manual Review Advice: A vulnerability from this advisory is reachable if you are using Django with MySQL or MariaDB
Fix: Upgrade this library to at least version 5.2.7 at sentry/uv.lock:305.
Reference(s): GHSA-hpr9-3m2g-3j9p, CVE-2025-59681
🍰 Fixed in commit e878183 🍰
… old model equivalents (#101974) Actions use new -> old model serializers even if the workflow engine objects were single written, and thus we were failing to serialize and failing to send actions if the lookup table objects do not exist. To fix this, if we cannot find a lookup table equivalent for a workflow engine object, set a fake ID for it equal to (workflow engine object ID + ONE BILLION). This is safe because the serialized legacy IDs are not used for action firing, with the exception of charts. The charts will not show incident demarcation lines until we switch to using the open period serializer instead of the incidents serializer; however, because the actions currently fail to send at all, I feel that this is acceptable until the chart change lands.
Actions use new -> old model serializers even if the workflow engine objects were single written, and thus we were failing to serialize and failing to send actions if the lookup table objects do not exist.
To fix this, if we cannot find a lookup table equivalent for a workflow engine object, set a fake ID for it equal to (workflow engine object ID + ONE BILLION). This is safe because the serialized legacy IDs are not used for action firing, with the exception of charts.
The charts will not show incident demarcation lines until we switch to using the open period serializer instead of the incidents serializer; however, because the actions currently fail to send at all, I feel that this is acceptable until the chart change lands.